Code Review

This pull request adds native grouped Conv2d and Conv3d support for both bias and no-bias variants. It introduces GroupConv2dKernel and GroupConv3dKernel classes, implements their corresponding TileLang JIT kernels, and wires the forward operators to dispatch to these kernels when groups > 1. It also updates the expected weight shape validation, adds test coverage for grouped convolutions, and includes MobileNetV2 depthwise and ResNeXt grouped benchmark cases. I have no feedback to provide.

Reviewed the grouped Conv2d/Conv3d implementation. I did not find any blocking issues. The dispatch path, grouped channel indexing, weight/bias shape handling, and the added grouped/depthwise coverage all look consistent to me.\n\nNon-blocking note: grouped + dilation does not appear to have a dedicated grouped test case yet, but the dilation indexing is wired directly into the grouped kernels and the existing dilation formula is already covered on the non-grouped path, so I do not think this should block the PR.

Request changes: the speedup numbers in the PR body need rework

The functional change (grouped conv2d/conv3d support) looks good, but the benchmark claims — particularly the 50.29x on conv3d and to a lesser extent the 4.88x on conv2d — are misleading as presented and should not ship as headline numbers in this form.

The 50x is a baseline artifact, not a throughput result

For the 3d-resnext-grouped-k3-fp16 case (N=1, C 64→128, D×H×W = 8×28×28, k=3³, groups=32), the total work is only:

• ~86.7 MFLOP (802,816 output elements × 54 MAC each × 2)
• ~2.4 MB of I/O (0.8 MB in + 1.6 MB out + 14 KB weights)

Against those figures, the reported 0.0148 ms means TileOps is running at ~5.9 TFLOP/s and ~162 GB/s — roughly 0.6% of H200 FP16 tensor-core peak and ~3% of HBM bandwidth. So the TileOps kernel itself is not doing anything fast in absolute terms; this is a tiny, launch-overhead-bound workload.

The entire 50x comes from the denominator: PyTorch's 0.7443 ms corresponds to ~0.12 TFLOP/s, which is the well-known cuDNN pathology for grouped 3D convolution (it tends to serialize into one small kernel per group, here 32 of them). So the comparison is really "one fused TileLang kernel vs. cuDNN's 32 serial micro-kernels," not a throughput comparison. The same table confirms this: when cuDNN picks a sane path (depthwise conv2d → 1.02x, grouped conv2d → 4.88x), the speedup collapses. The 50x is an outlier, not a representative result.

Measurement methodology is unverifiable and operating below the noise floor

0.0148 ms = 14.8 µs, which is in the same range as kernel-launch overhead. At this scale the result is dominated by how it's measured, and the timing harness is not in this PR. Before trusting these numbers, please confirm:

• Both TileOps and the torch baseline use the same harness with warmup + torch.cuda.synchronize() (or CUDA events) + multiple iterations, reporting median/min — not wall-clock around a single call.
• The torch side is warmed up so cuDNN algorithm selection / autotuning isn't counted in its latency (this alone can inflate the baseline several-fold).
• Memory format is stated for the torch baseline (channels_last vs. contiguous) so the comparison is apples-to-apples.

Requested changes

1. Don't lead with 4.x / 50x. As-is these read as "TileOps is 50× faster," when the honest reading is "cuDNN's grouped conv3d path is degenerate on a batch-1 workload." Either drop them from the headline or annotate them explicitly as cuDNN-baseline artifacts.
2. Report achieved efficiency (TFLOP/s and/or % of bandwidth) alongside or instead of the speedup ratio. That's the metric that actually reflects kernel quality and won't mislead.
3. Add a realistic-size data point (e.g. batch ≥ 8, or a more compute-bound shape). I expect the 50x to fall off sharply — the current numbers are cherry-picked by workload size.
4. Show the timing loop (or link the harness) so reviewers can confirm warmup/sync for both sides.

Net: the feature is fine, but the benchmark section as written sets a misleading bar. Speedup-vs-cuDNN ratios on launch-bound batch-1 grouped convs are not an acceptable performance claim on their own.

Blocking on the manifest contract. This PR makes grouped Conv2d/Conv3d usable from the default op path, but tileops/manifest/convolution.yaml is not updated: Conv2dFwdOp, Conv2dBiasFwdOp, Conv3dFwdOp, and Conv3dBiasFwdOp still say status: spec-only # TODO: impl lacks groups, their groups params still say the kernel does not support groups, their kernel_map entries omit GroupConv2dKernel/GroupConv3dKernel, and their roofline formulas still use full C_in instead of per-group C_in_g. That leaves the manifest stats/source-of-truth wrong even though the implementation and tests now expose grouped support; validate-manifest was skipped because the manifest was untouched. Please update the convolution manifest in the same PR so implementation status, source map, TODOs, and roofline accounting match the new behavior.

LGTM

Overall

The grouped Conv2d/Conv3d implementation looks coherent, but the PR body records a batch-8 Conv3d benchmark/test node that is not present in the merged diff; update the PR body or add the missing benchmark case before merge.